doc: clarify socket and keepAliveTimeout interaction#25748
doc: clarify socket and keepAliveTimeout interaction#25748inversion wants to merge 1 commit intonodejs:masterfrom
Conversation
nodejs-github-bot
added
doc
inversion
force-pushed
the
doc-clarify-http-keepAliveTimeout
branch
from
344dfc6 to
4696232
Compare
|
The subsystem prefix in commit messages for doc-only changes should be |
doc/api/http.md
Outdated
There was a problem hiding this comment.
| If `socket.setTimeout()` is called here, note that the timeout will | |
| If `socket.setTimeout()` is called here, the timeout will |
There was a problem hiding this comment.
@Trott Are we landing as is or can we add this fix on landing?
There was a problem hiding this comment.
@inversion are you ok with @Trott suggestion? If so please accept it so this can land.
Thank you.
There was a problem hiding this comment.
My change is non-blocking. It's not crucial that note that be removed. (It can also be removed by someone else who is landing this--just push to the branch. Or it can be removed in a subsequent PR as I imagine there are lots of opportunities for cleanup in the doc file.)
|
Hi, @inversion! Welcome and thanks for the pull request! Sorry to hear that this behavior caused you some grief! /ping @nodejs/http to confirm that the behavior described here is correct and is currently undocumented. /ping @nodejs/documentation Maybe there's a way to simplify or clarify the wording here a bit? Maybe a small piece of sample code with comments indicating the values and when they change? Or is that too much documentation real estate to describe what is likely an edge case? |
I'll fix this, probably worth adding it to https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines |
Socket timeouts set using the `connection` event are replaced by server.keepAliveTimeout when a response is handled.
Trott
force-pushed
the
doc-clarify-http-keepAliveTimeout
branch
from
4696232 to
5f761e9
Compare
|
I fixed up the commit message subsystem, rebased against master, and force-pushed. Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2423/ |
Trott
changed the title
Trott
added
the
author ready
|
Thanks very much @Trott ! |
|
@nodejs/collaborators This could use another review. |
`Socket.prototype.setTimeout()` clears the previous timer before setting a new one. Refs: nodejs#25748 (comment)
`Socket.prototype.setTimeout()` clears the previous timer before setting a new one. Refs: #25748 (comment) PR-URL: #25928 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
`Socket.prototype.setTimeout()` clears the previous timer before setting a new one. Refs: #25748 (comment) PR-URL: #25928 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 02601c2. |
Socket timeouts set using the `'connection'` event are replaced by `server.keepAliveTimeout` when a response is handled. PR-URL: #25748 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Socket timeouts set using the `'connection'` event are replaced by `server.keepAliveTimeout` when a response is handled. PR-URL: #25748 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
7 participants
doc: clarify socket and keepAliveTimeout interaction
Socket timeouts set using the
connectionevent are replaced byserver.keepAliveTimeout when a response is handled.
Socket timeouts set in the
connectionevent are replaced after a response is sent in theresOnFinishhandlerIn this case, the socket timeout is 60 seconds until a response is sent, at which point it becomes 5 seconds (the server.keepAliveTimeout default). This bit me when working with load balancers that maintain long-lived connections.
I have added a note to the
connectionevent docs to the effect that is preferred to useserver.keepAliveTimeout.Checklist